Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Sigv4A support for the orchestrator #2890

Closed
wants to merge 15 commits into from
Closed

Add Sigv4A support for the orchestrator #2890

wants to merge 15 commits into from

Conversation

Velfi
Copy link
Contributor

@Velfi Velfi commented Jul 31, 2023

This includes unit tests but no e2e test. I'll work on that in a separate PR, since this one is already quite busy.

Most of the 'noise' comes from the CRT sigv4a test files I added. The were added in their own commit so that it'll be easier to filter them out during review. You can ignore all files in aws/rust-runtime/aws-sigv4/aws-sig-v4a-test-suite/.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@Velfi Velfi requested a review from a team as a code owner July 31, 2023 20:39
Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this!

/// Auth scheme ID for SigV4a.
pub const SCHEME_ID: AuthSchemeId = AuthSchemeId::new("sigv4a");

struct EndpointAuthSchemeConfig {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you foresee these supporting types (e.g., EndpointAuthSchemeConfig, SigV4SigningError, HttpSignatureType, SigningOptions, etc) ever deviating from SigV4's? If not, should we consolidate them and their shared logic so that the only difference is in the scheme ID and the signer itself?

Comment on lines 275 to 276
let signature = match params.signature_version {
SignatureVersion::V4 => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider breaking out functions for the different versions

val normalizeUrlPath = !disableUriPathNormalization(codegenContext.serviceShape)
rustTemplate(
"""
let mut signing_options = #{SigningOptions}::default();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking we should combine the V4 and V4a decorators so that this signing option initialization only needs to happen once (assuming the supporting types are merged).

@@ -279,7 +279,6 @@ fun rewritePathDependency(line: String): String {
}

tasks.register<Copy>("copyAllRuntimes") {
dependsOn("smithyBuildJar")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commenting for posterity: this makes the relocate runtime tasks run without regenerating the entire SDK.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:-) thanks for fixing

@Velfi Velfi requested a review from a team as a code owner August 8, 2023 21:48
Comment on lines +12 to +14
let _ = v4a::generate_signing_key(
"AKIAIOSFODNN7EXAMPLE",
"wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you need to blackbox these otherwise it can optimize away the entire function

@Velfi
Copy link
Contributor Author

Velfi commented Aug 10, 2023

Closing this in favor of the sub-PRs
#2911
#2913

  • a yet-to-be-published PR for the Sigv4A support part

@Velfi Velfi closed this Aug 10, 2023
@Velfi Velfi reopened this Aug 21, 2023
@Velfi Velfi closed this Aug 21, 2023
@landonxjames landonxjames deleted the zhessler-sigv4a branch January 13, 2025 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants